Simplify dotTrace diagnoser, dedupe common code #2558
Conversation
… by including from shared project
|
Could you use shared project for this?
|
|
TIL, did not know that existed 😄 I wanted to test that out now but I'm not sure if it's possible to create that project type for me since I'm working on Linux atm. .NET SDK, Rider and VSCode C# DevKit all seem to be missing support for creating this project type. Reading up on this issue: dotnet/sdk#2511 I could try out shared projects, but I would have to hand-code it unless someone on a Windows PC could scaffold it for me. What do you think? |
|
Ah, I didn't realize that was legacy project type. I think just using the folder approach is good. |
| { | ||
| public class Progress : IProgress<double> | ||
|
|
||
| internal sealed class Progress : IProgress<double> |
There was a problem hiding this comment.
This was public before. Is it ok to remove it from public API? @AndreyAkinshin
There was a problem hiding this comment.
True, I forgot to mention that. I figured since it's not used by any configuration or anything it makes sense to not expose it? Users could have referenced it for some reason, but not because of any interaction with BenchmarkDotNet AFAICT.
If we keep it public I probably have to do something about the namespace, so that both packages don't expose the same type in the same namespace, unless that's not a big deal since we don't expect anyone to import BenchmarkDotNet.JetBrains regardless of which package is used?
...JetBrains/BenchmarkDotNet.Diagnostics.dotMemory/BenchmarkDotNet.Diagnostics.dotMemory.csproj
Outdated
Show resolved
Hide resolved
src/JetBrains/BenchmarkDotNet.Diagnostics.dotTrace/BenchmarkDotNet.Diagnostics.dotTrace.csproj
Outdated
Show resolved
Hide resolved
|
@martinothamar thanks for the PR! Indeed, it makes sense to unify dotTrace/dotMemory diagnosers and eliminate code duplication. However, the presented approach inherits the disadvantages of the poor design of the original diagnosers. Both were born as prototypes. Now, it's time to refactor them. I have come up with a better solution for this problem, see #2627. |
Similar approach to #2549 for simplifying
DotTraceDiagnoser.Also deduped some code by creating a shared project (using
<Compile Include="..." />to avoid another package).Pulled this code into
JetBrainsfolder so that the projects are ordered next to eachother in file explorers.Made sure to update build script and references to include the new folder.
Other options: